Conversation
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideHardens the FileStore.safePath implementation to use absolute paths and filepath.Rel for robust directory containment checks, addressing a path traversal/code scanning alert without changing the public API. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
skyoo2003
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider resolving
fs.baseDirto an absolute path once inNewFileStoreand storing it on the struct, rather than callingfilepath.Abson everysafePathinvocation to avoid repeated filesystem/OS work on hot paths. - Since
joinedis already built frombaseAbs, callingfilepath.Absoncleanedis redundant; you can simplify by dropping the secondAbscall and usingcleaneddirectly as the candidate path forReland the return value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider resolving `fs.baseDir` to an absolute path once in `NewFileStore` and storing it on the struct, rather than calling `filepath.Abs` on every `safePath` invocation to avoid repeated filesystem/OS work on hot paths.
- Since `joined` is already built from `baseAbs`, calling `filepath.Abs` on `cleaned` is redundant; you can simplify by dropping the second `Abs` call and using `cleaned` directly as the candidate path for `Rel` and the return value.
## Individual Comments
### Comment 1
<location path="internal/services/s3/store.go" line_range="41-45" />
<code_context>
+ return "", fmt.Errorf("resolve candidate path: %w", err)
+ }
+
+ rel, err := filepath.Rel(baseAbs, candidateAbs)
+ if err != nil {
+ return "", fmt.Errorf("resolve relative path: %w", err)
+ }
+ if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+ return "", fmt.Errorf("path traversal detected: %s", candidateAbs)
}
</code_context>
<issue_to_address>
**🚨 issue (security):** Path traversal check does not account for symlinks inside the base directory.
`Abs` + `Rel` blocks `..` traversal but not symlinks under `baseAbs` that point outside it, so the isolation guarantee can be broken. For stronger confinement, resolve symlinks (e.g. with `filepath.EvalSymlinks`) on `baseAbs` and/or `candidateAbs` and run the `Rel` check on those resolved paths.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Keep the HEAD version which uses absolute path resolution for stronger path traversal protection.
Resolve baseDir symlinks once in NewFileStore via EvalSymlinks, remove per-call filepath.Abs overhead in safePath, and resolve candidate symlinks before containment check to prevent escape via symlinks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/skyoo2003/devcloud/security/code-scanning/13
To fix this safely without changing functionality, harden
safePathininternal/services/s3/store.goso it:baseDirand candidate path to absolute paths (filepath.Abs).filepath.Rel(baseAbs, candidateAbs)to verify containment.rel == ".."or starts with".."+separator(escape).This is the best single fix because it preserves current API behavior and all call sites, while replacing a fragile prefix check with a standard, cross-platform path containment validation pattern. No provider-side routing changes are required for this specific sink hardening.
Suggested fixes powered by Copilot Autofix. Review carefully before merging.